feat: convert `BackendTLSPolicies` into service annotations #6753

mlavacca · 2024-12-02T17:05:52Z

What this PR does / why we need it:

This PR completes the logic needed to implement BackendTLSPolicy. The following 3 main parts have been implemented:

completed the BackendTLSPolicy controller by validating the policy and setting the Accepted condition accordingly.
completed the dataplane part, where all the BackendTLSPolicy features have been converted into the proper set of service annotations.
implemented the configMap controller, as the CACertificates referenced by the policies need to be set in configMaps by the specification.

Which issue this PR fixes:

Part of #6631

Special notes for your reviewer:

Integration and env tests are still missing as the PR size is already big enough. The next PR will be the last one, where I'll implement such tests and close #6631.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

codecov · 2024-12-02T17:27:47Z

Codecov Report

Attention: Patch coverage is 46.30435% with 247 lines in your changes missing coverage. Please review.

Project coverage is 79.4%. Comparing base (f343327) to head (cc5ad38).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/dataplane/translator/ingressrules.go	20.0%	54 Missing and 2 partials ⚠️
.../controllers/configuration/configmap_controller.go	35.0%	45 Missing and 5 partials ⚠️
...rnal/controllers/gateway/backendtlspolicy_utils.go	51.8%	36 Missing and 2 partials ⚠️
internal/controllers/reference/reference.go	46.1%	25 Missing and 3 partials ⚠️
internal/store/store.go	39.1%	25 Missing and 3 partials ⚠️
...controllers/gateway/backendtlspolicy_controller.go	11.1%	24 Missing ⚠️
internal/annotations/annotations.go	34.7%	15 Missing ⚠️
internal/store/fake_store.go	42.8%	3 Missing and 1 partial ⚠️
internal/dataplane/translator/translate_cacerts.go	91.1%	2 Missing and 1 partial ⚠️
internal/controllers/gateway/gateway_controller.go	87.5%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #6753     +/-   ##
=======================================
+ Coverage   77.7%   79.4%   +1.7%     
=======================================
  Files        207     208      +1     
  Lines      24681   25051    +370     
=======================================
+ Hits       19181   19911    +730     
+ Misses      4523    4130    -393     
- Partials     977    1010     +33

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mlavacca · 2024-12-06T16:27:39Z

Note for the reviewers: take a look at the PR description where I've put some notes about what has been implemented and what's still missing.

randmonkey · 2024-12-10T09:42:32Z

internal/controllers/configuration/configmap_controller.go

+			CacheSyncTimeout: r.CacheSyncTimeout,
+		}).
+		Watches(&corev1.ConfigMap{},
+			&handler.EnqueueRequestForObject{},


Should we add an explicit limit on ConfigMaps to watch by labels to prevent potential excessive memory consumption to look at all ConfigMaps, or provide an option to add such filter, as Jintao did for Secrets?
The total number of ConfigMaps could be very large and most of them would not be relevant.

Sounds good, I'll add it 👍

internal/controllers/gateway/backendtlspolicy_utils.go

internal/controllers/reference/reference.go

All the BackendTLSPolicies are converted into a set of annotations that are already supported by KIC. Signed-off-by: Mattia Lavacca <[email protected]>

Signed-off-by: Mattia Lavacca <[email protected]>

CHANGELOG.md

internal/controllers/gateway/backendtlspolicy_utils.go

internal/controllers/reference/reference.go

internal/dataplane/translator/translate_cacerts.go

programmer04 · 2024-12-10T11:45:50Z

internal/dataplane/translator/translate_cacerts.go

+		// Allow the certificate key to be named either "cert" or "ca.crt"
+		caCertbytes, certExists := certConfigMap.Data["cert"]
+		if !certExists {
+			caCertbytes, certExists = certConfigMap.Data["ca.crt"]
+			if !certExists {
+				relatedObjects := getPluginsAssociatedWithCACertSecret(certID, t.storer)
+				relatedObjects = append(relatedObjects, certConfigMap.DeepCopy())
+				t.registerTranslationFailure(fmt.Sprintf(`invalid configmap CA certificate %s/%s, neither "cert" nor "ca.crt" key exist`, certConfigMap.Namespace, certConfigMap.Name), relatedObjects...)
+				continue
+			}
+		}


This is basically the same as 37:47. Maybe it's worth having a common helper for this? WDYT?

Yes, I agree. I've refactored the function in 4ab2f38

internal/store/fake_store.go

internal/store/store.go

internal/annotations/annotations.go

examples/ingress-upstream-tls.yaml

internal/annotations/annotations.go

internal/controllers/reference/reference.go

internal/dataplane/translator/ingressrules.go

internal/controllers/reference/reference.go

Co-authored-by: Jakub Warczarek <[email protected]> Co-authored-by: Patryk Małek <[email protected]>

Co-authored-by: Jakub Warczarek <[email protected]>

Signed-off-by: Mattia Lavacca <[email protected]>

CHANGELOG.md

docs/cli-arguments.md

Signed-off-by: Mattia Lavacca <[email protected]>

CHANGELOG.md

internal/controllers/configuration/object_references.go

internal/controllers/gateway/backendtlspolicy_controller.go

internal/controllers/gateway/gateway_controller.go

internal/controllers/reference/reference.go

pull-request-size bot added the size/M label Dec 2, 2024

mlavacca force-pushed the backendtlspolicy-customize-services branch from 03b4d14 to 9d8d427 Compare December 2, 2024 17:06

mlavacca changed the title ~~feat: convert BackendTLSPolicies into service anns~~ feat: convert BackendTLSPolicies into service annotations Dec 2, 2024

pull-request-size bot added size/L and removed size/M labels Dec 5, 2024

mlavacca force-pushed the backendtlspolicy-customize-services branch from 250d107 to 343d922 Compare December 5, 2024 10:59

pull-request-size bot added size/XL and removed size/L labels Dec 5, 2024

mlavacca force-pushed the backendtlspolicy-customize-services branch from c74c408 to 7800ee4 Compare December 5, 2024 11:23

pull-request-size bot added size/XXL and removed size/XL labels Dec 5, 2024

mlavacca force-pushed the backendtlspolicy-customize-services branch 7 times, most recently from de488b5 to c095f6b Compare December 6, 2024 15:51

mlavacca marked this pull request as ready for review December 6, 2024 16:26

mlavacca requested a review from a team as a code owner December 6, 2024 16:26

mlavacca force-pushed the backendtlspolicy-customize-services branch from 7453d19 to ef08584 Compare December 9, 2024 11:32

mlavacca mentioned this pull request Dec 9, 2024

test: Backendtlspolicy integration test #6799

Draft

1 task

randmonkey reviewed Dec 10, 2024

View reviewed changes

mlavacca added 3 commits December 10, 2024 12:42

feat: convert BackendTLSPolicies into service anns

f2d83d6

All the BackendTLSPolicies are converted into a set of annotations that are already supported by KIC. Signed-off-by: Mattia Lavacca <[email protected]>

chore: CHANGELOG entry added

3862656

Signed-off-by: Mattia Lavacca <[email protected]>

address review comments

ecd4668

Signed-off-by: Mattia Lavacca <[email protected]>

mlavacca force-pushed the backendtlspolicy-customize-services branch from 8e87773 to ecd4668 Compare December 10, 2024 11:43

programmer04 reviewed Dec 10, 2024

View reviewed changes

pmalek reviewed Dec 10, 2024

View reviewed changes

mlavacca and others added 3 commits December 10, 2024 15:30

Apply suggestions from code review

17feeb9

Co-authored-by: Jakub Warczarek <[email protected]> Co-authored-by: Patryk Małek <[email protected]>

Update internal/controllers/reference/reference.go

b6c8750

Co-authored-by: Jakub Warczarek <[email protected]>

address reviews comments

435c38c

Signed-off-by: Mattia Lavacca <[email protected]>

mlavacca force-pushed the backendtlspolicy-customize-services branch 4 times, most recently from 6159e82 to 1a392ac Compare December 10, 2024 16:33

mlavacca added 4 commits December 10, 2024 17:41

add secret-label-selector

dfff875

Signed-off-by: Mattia Lavacca <[email protected]>

refactor getCACerts

63cb6b0

Signed-off-by: Mattia Lavacca <[email protected]>

make lint

08cfa48

Signed-off-by: Mattia Lavacca <[email protected]>

refactor UpdateReferencesToSecret and removeOutdatedReferencesToSecret

de81f12

Signed-off-by: Mattia Lavacca <[email protected]>

mlavacca force-pushed the backendtlspolicy-customize-services branch from 1a392ac to de81f12 Compare December 10, 2024 16:41

pmalek reviewed Dec 10, 2024

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

docs/cli-arguments.md Outdated Show resolved Hide resolved

add default for --configmap-label-selector

ed77475

Signed-off-by: Mattia Lavacca <[email protected]>

mlavacca requested review from randmonkey, pmalek and programmer04 December 10, 2024 20:09

pmalek assigned mlavacca Dec 10, 2024

pmalek added this to the KIC v3.4.x milestone Dec 10, 2024

pmalek reviewed Dec 10, 2024

View reviewed changes

Apply suggestions from code review

cc5ad38

pmalek approved these changes Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: convert `BackendTLSPolicies` into service annotations #6753

feat: convert `BackendTLSPolicies` into service annotations #6753

mlavacca commented Dec 2, 2024 •

edited by pmalek

Loading

codecov bot commented Dec 2, 2024 •

edited

Loading

mlavacca commented Dec 6, 2024 •

edited

Loading

randmonkey Dec 10, 2024

mlavacca Dec 10, 2024

mlavacca Dec 10, 2024

programmer04 Dec 10, 2024

mlavacca Dec 10, 2024

feat: convert BackendTLSPolicies into service annotations #6753

Are you sure you want to change the base?

feat: convert BackendTLSPolicies into service annotations #6753

Conversation

mlavacca commented Dec 2, 2024 • edited by pmalek Loading

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

mlavacca commented Dec 6, 2024 • edited Loading

randmonkey Dec 10, 2024

Choose a reason for hiding this comment

mlavacca Dec 10, 2024

Choose a reason for hiding this comment

mlavacca Dec 10, 2024

Choose a reason for hiding this comment

programmer04 Dec 10, 2024

Choose a reason for hiding this comment

mlavacca Dec 10, 2024

Choose a reason for hiding this comment

feat: convert `BackendTLSPolicies` into service annotations #6753

feat: convert `BackendTLSPolicies` into service annotations #6753

mlavacca commented Dec 2, 2024 •

edited by pmalek

Loading

codecov bot commented Dec 2, 2024 •

edited

Loading

mlavacca commented Dec 6, 2024 •

edited

Loading